Ported reducer and p2p effects in node #784
Conversation
0xMimir
force-pushed
the
feat/reducer-port
branch
from
ae65856 to
6a2d95d
Compare
0xMimir
force-pushed
the
feat/reducer-port
branch
from
6a2d95d to
d531b0d
Compare
| pub on_p2p_connection_outgoing_success: OptionalCallback<RequestId<RpcIdType>>, | ||
| /// Callback for [`P2pConnectionOutgoingAction::Error`] | ||
| pub on_p2p_connection_outgoing_error: OptionalCallback<(RpcId, P2pConnectionOutgoingError)>, | ||
| /// Callback for [`P2pConnectionOutgoingAction::Success`] |
There was a problem hiding this comment.
@0xMimir that would be P2pConnectionOutgoingAction::Init right? Basically, this callback will be called as a result of the Init action.
So on the caller which now has:
store.dispatch(P2pConnectionOutgoingAction::Init {
opts,
rpc_id: Some(rpc_id),
});if we were to add the callbacks, we would have:
store.dispatch(P2pConnectionOutgoingAction::Init {
opts,
rpc_id: Some(rpc_id),
on_success: ....this callback....,
on_error: ...the error callback...,
});There was a problem hiding this comment.
@0xMimir I see this still contains the previous comments, so let me clarify. The idea is that it should mention the original action to which it relates (from the point of view of the caller). Like in the example above, the caller will never dispatch Success or Error, he will dispatch Init, so this is a callback for Init. But it is no big deal, just a nice-to-have to make it easier to modification I mentioned above in the future.
|
|
||
| #[derive(Serialize, Deserialize, Debug, Clone, ActionEvent)] | ||
| #[action_event(level = debug)] | ||
| pub enum P2pCallbacksAction { |
There was a problem hiding this comment.
Are these callbacks RPC related only?
In this case we could change the name to something like P2pRpcCallbackAction
There was a problem hiding this comment.
Callbacks are optional because of p2p tests, P2pCallbacksAction is currently only RPC related, but it doesn't have to be.
| } | ||
| } | ||
| RpcAction::P2pConnectionIncomingPending { .. } => {} | ||
| RpcAction::P2pConnectionIncomingAnswerReady { |
There was a problem hiding this comment.
This is a case of a new action that is required only because you need to dispatch two different actions from the callback?
There was a problem hiding this comment.
Yes, but I can remove it, set callback to RpcAction::P2pConnectionIncomingRespond and dispatch P2pConnectionIncomingAction::AnswerSendSuccess after callback in p2p crate
There was a problem hiding this comment.
No, it is fine, was just asking to make sure I was interpreting it correctly.
...c/transition_frontier/sync/ledger/snarked/transition_frontier_sync_ledger_snarked_reducer.rs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| TransitionFrontierAction::RpcRespondBestTip { .. } => { | ||
| state.transition_frontier.best_tip().is_some() |
There was a problem hiding this comment.
what happens if we don't have a best tip? we just never answer? doesn't feel right. This is a bug in the original code, not introduced in the PR, but just now noticed it.
There was a problem hiding this comment.
Added a bunch of comments. Some things are probably out of scope to the PR but it would be good to at least get a TODO/FIXME comment for those so that we don't forget later. Same for adding callbacks to the actions directly associated to them, that is for another iteration (comments mentioning the associated actions is good enough for now, unless you find that it is easy enough to do in this PR).
I would like to have the new actions added to the transition frontier moved out from there and into the p2p related callbacks, they feel out of place in the transition frontier and introduce more inter-component dependencies, which we can reduce.
0xMimir
force-pushed
the
feat/reducer-port
branch
2 times, most recently
from
b647e1e to
a666c66
Compare
0xMimir
force-pushed
the
feat/reducer-port
branch
from
a666c66 to
f43b87d
Compare
3 participants
No description provided.